feat(sdk-core): surface pendingApprovalId from TSS message signing by nvjsr · Pull Request #9858 · BitGo/BitGoJS

nvjsr · 2026-09-30T04:44:17Z

Ticket

https://linear.app/bitgo/issue/DEFI-1014

Problem

The message-signing policy (DEFI-653) parks signMessage/signTypedStructuredData txRequests behind a pending approval, but the SDK's message-sign paths never inspected the txRequest state:

They ran all MPC signing rounds against a parked request and failed at the final send with the wrapped failed to sign typed data … Expected transaction request to be in state pendingDelivery but it is in state pendingApproval — the original E2E failure (Error 1). The string-wrap also destroyed the ApiResponseError structure, so no UI guard could recover the pending approval id.
After approval, the only retry path (signAndSendTxRequest) signs through the transaction path and crashes on message-only requests: TypeError: Cannot read properties of undefined (reading 'unsignedTx') (Error 2).

The tx-send flow already had the reference implementation: prebuildAndSignTransaction early-returns the PA-parked txRequest (isPendingApprovalTxRequestFull).

Changes

PA early-return in message signing — signMessageTss and signTypedDataTss check the created/fetched txRequest after create-or-fetch (both the fresh-create and txRequestId resume branches) and return { coin, messageRaw, txRequestId, pendingApprovalId } instead of attempting MPC signing. Mirrors the tx flow's early return.
Type contract — discriminated union on the txRequest state (addresses review feedback) — SignedMessage = { state: 'pendingApproval'; pendingApprovalId; … } | { state: 'delivered'; txHash: string; signature: string; … }. The discriminator is the wallet-platform txRequest state (message requests go pendingApproval → pendingDelivery → delivered; the txRequest-level signed state never occurs for messages). BREAKING: consumers previously reading txHash/signature unguarded must now narrow on state === 'delivered' — the compile error lands exactly where an unguarded parked-result read would misbehave. signMessage/signTypedData/signAndSendMessageTxRequest all resolve parked requests to the pendingApproval variant, so callers surface the approval instead of treating it as a failure. Callers in bitgo-ui (#12219) and retail-web (#10305) narrow on state in the same pass.
signAndSendMessageTxRequest({ txRequestId, walletPassphrase }) — new public method; message-signing twin of signAndSendTxRequest. Signs a message txRequest through the message path (WP-persisted messages[0].messageEncoded as bufferToSign) so the tx-request details page can resume a parked request after approval without the unsignedTx TypeError. Resolves with pendingApprovalId if the request is still parked.
Rethrow original errors — signMessageTss/signTypedDataTss no longer wrap errors in failed to sign message/typed data …, so ApiResponseError structure (incl. the 409 TxRequestPendingApprovalError contract from DEFI-954 / WP PR 62988) survives for downstream guards.

Test evidence

npx mocha test/v2/unit/wallet.ts --grep "Message Signing|Typed Data|policy flow" → 48 passing (7 new):
- fresh signMessage/signTypedData parked on create → pendingApprovalId, no signing attempted
- resume-by-txRequestId parked → pendingApprovalId, no signing attempted
- signAndSendMessageTxRequest on a pendingDelivery message request → signs, returns txHash/signature
- signAndSendMessageTxRequest on a still-parked request → pendingApprovalId, no signing attempted
- non-TSS wallet → rejected
full test/v2/unit/wallet.ts suite → exit 0, byte-identical output to master baseline (incl. pre-existing Canton trace noise)
tsc --noEmit in sdk-core → 0 errors; prettier clean

Dependencies / ordering

Pairs with DEFI-952 (WP PR 62987 — share-ingestion gate, security) and DEFI-954 (WP PR 62988 — 409 contract): both should merge before this releases; this SDK becomes the primary parked-as-data path, the 409 remains the fallback for older SDKs.
Blocks DEFI-1015 (bitgo-ui + retail-web wiring).
No wallet-platform changes required.

Contract

SignedMessage consumers: txHash/signature are string | undefined after this change — callers must check pendingApprovalId first (parked) or use txHash/signature (signed). The no-policy path is unchanged: a pendingDelivery request signs exactly as before.

linear-code · 2026-09-30T04:44:30Z

DEFI-1014

github-actions · 2026-09-30T05:08:45Z

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

mullapudipruthvik · 2026-09-30T05:26:55Z

@claude

github-actions · 2026-09-30T05:27:18Z

Claude finished @mullapudipruthvik's task —— View job

Code Review for TSS Message Signing Pending Approval Changes

Todo List:

Read and analyze the core implementation changes in wallet.ts
Review interface changes in iBaseCoin.ts and iWallet.ts
Examine test coverage and quality in wallet.ts tests
Review XDC-specific changes for consistency
Check for potential security or edge case issues
Provide comprehensive review feedback

Overall Assessment ✅

This is a well-architected solution that systematically addresses the message signing policy flow issues. The implementation follows existing patterns and maintains backward compatibility while fixing critical user experience problems.

Detailed Review

🟢 Strengths

Excellent Problem Analysis: The PR clearly identifies two specific errors and addresses both systematically.
Consistent Architecture: The solution mirrors the existing transaction flow's pending approval handling (prebuildAndSignTransaction), maintaining consistency across the codebase.
Type Safety: The interface changes are well-designed:
- SignedMessage.txHash/signature made optional appropriately
- pendingApprovalId field clearly documented (lines 611-618 in iBaseCoin.ts)
- New SignAndSendMessageTxRequestOptions interface is minimal and focused
Comprehensive Error Handling:
- Removes error wrapping to preserve ApiResponseError structure (lines 5470-5475, 5543-5548)
- This is crucial for downstream error classification and UI handling
Robust Implementation:
- Both signMessageTss and signTypedDataTss have identical pending approval checks
- signAndSendMessageTxRequest method provides clean resume path
- Proper validation of message data structures
Strong Test Coverage: The message signing tests are comprehensive, covering:
- Fresh requests parked on create
- Resume-by-txRequestId scenarios
- signAndSendMessageTxRequest success and parked states
- Non-TSS wallet rejection

🟡 Minor Observations

Line 5465 Inconsistency: In signMessageTss, both txHash and signature are set to the same value (signedMessageRequest.messages[0].txHash). This seems intentional based on existing patterns but worth noting.
XDC Integration: The XDC changes properly handle the new optional signature fields with clear error messages when approval is required.

🟢 Security Assessment

No security vulnerabilities identified
Maintains existing access controls and validation
Error handling preserves important security context
No credential exposure risks

🟢 Code Quality

Follows existing TypeScript and BitGoJS conventions
Proper use of assertions and error handling
Clear, descriptive comments explaining the policy flow logic
Maintains consistency with transaction signing patterns

Recommendation: APPROVE ✅

This PR successfully addresses the stated problems with a clean, well-tested solution that maintains architectural consistency. The implementation will significantly improve the user experience for message signing with policies enabled.

Job Run | Branch

mohammadalfaiyazbitgo · 2026-10-01T18:49:56Z

-  txHash: string;
-  signature: string;
+  // Absent when the sign request was parked behind a pending approval (see pendingApprovalId).
+  txHash?: string;


this is a breaking change. Please document it as such.

Addressed in f10bf80 — and gone further than documentation: SignedMessage is now a discriminated union on the wallet-platform txRequest state:

type SignedMessage = | { state: 'pendingApproval'; pendingApprovalId: string; txRequestId; messageRaw; … } | { state: 'delivered'; txHash: string; signature: string; txRequestId; messageRaw; … };

txHash/signature are required on the delivered variant, so consumers narrow on state rather than handling string | undefined — the compile error now lands exactly where an unguarded parked-result read would have misbehaved. BREAKING is documented on the union. Consumers in bitgo-ui (#12219) and retail-web (#10305) were updated in the same pass to narrow on state.

zahin-mohammad · 2026-10-01T19:19:18Z

-  txHash: string;
-  signature: string;
+  // Absent when the sign request was parked behind a pending approval (see pendingApprovalId).
+  txHash?: string;


zahin-mohammad · 2026-10-01T19:20:22Z

+   * absent, and the caller should surface the approval instead of treating
+   * this as a failure. After approval, re-sign via signAndSendMessageTxRequest.
+   */
+  pendingApprovalId?: string;


Why are we relying on this instead of the txRequest state? The txRequest state should clearly say what stage the txRequest is in, you shouldn't have to infer this from the presence of an id..

Fair point — implemented in f10bf80: the discriminator is now the txRequest state itself. state: 'pendingApproval' | 'delivered' (message requests go pendingApproval → pendingDelivery → delivered; the txRequest-level signed state never occurs for message requests, so it is deliberately not part of the union). Detection was already state-based internally (isPendingApprovalTxRequestFull: apiVersion === 'full' && state === 'pendingApproval'); now the surfaced contract carries the state too, so no consumer has to infer anything from id presence.

zahin-mohammad · 2026-10-01T19:21:38Z

  }

+  /**
+   * Signs (and, for full requests, delivers) the message of a message-sign


(and, for full requests, delivers)

why are we still supporting txRequest lite for this flow? Please make this flow full only, lite is a deprecated flow, it shouldn't be receiving new features.

Good catch — the implementation never had a lite branch (message-sign requests are full-only; the deprecated lite flow never carries messages). The wording was inherited from signAndSendTxRequest's doc and was doubly misleading ('delivers' was also wrong — the method only signs; delivery completes server-side once signature shares are ingested). Doc comment rewritten in f10bf80 to state full-only explicitly.

The message-signing policy (DEFI-653) parks signMessage/signTypedStructuredData txRequests behind a pending approval, but the SDK's message-sign paths never inspected the txRequest state: they ran all MPC signing rounds against a parked request and failed at the final send with a wrapped 'Expected transaction request to be in state pendingDelivery' error, leaving the UI unable to recover the pending approval id. After approval, the only retry path (signAndSendTxRequest) signs through the transaction path and crashes on message-only requests (transactions[0].unsignedTx of undefined). - signMessageTss/signTypedDataTss: after create/fetch, return the pendingApprovalId (with txRequestId) when the txRequest is parked behind a pending approval instead of attempting to sign, mirroring the tx flow's early return in prebuildAndSignTransaction - SignedMessage: txHash/signature are now optional and a pendingApprovalId field marks the parked result; callers should treat a present pendingApprovalId as awaiting-approval data, not a failure - new signAndSendMessageTxRequest({ txRequestId, walletPassphrase }): signs a message txRequest through the message path (WP-persisted messageEncoded as bufferToSign), for resuming a parked request after approval; resolves with the pendingApprovalId if the request is still parked - rethrow the original signing errors instead of wrapping them in 'failed to sign message/typed data ...' so downstream callers can classify ApiResponseError results (e.g. the TxRequestPendingApprovalError 409) - sdk-coin-xdc: signXdcKycMessage fails loudly when the KYC sign request is parked behind a pending approval instead of returning an unusable signature-less result Refs: DEFI-1014

nvjsr requested review from a team as code owners September 30, 2026 04:44

nvjsr requested review from lokesh-bitgo and zahin-mohammad September 30, 2026 04:44

nvjsr force-pushed the nehakumari/defi-1014-sdk-core-surface-pendingapprovalid-from-tss-message-signing branch from 8257880 to 913b508 Compare September 30, 2026 05:00

venkateshv1266 previously approved these changes Oct 1, 2026

View reviewed changes

mohammadalfaiyazbitgo requested changes Oct 1, 2026

View reviewed changes

zahin-mohammad reviewed Oct 1, 2026

View reviewed changes

nvjsr dismissed venkateshv1266’s stale review via f10bf80 October 5, 2026 04:02

nvjsr force-pushed the nehakumari/defi-1014-sdk-core-surface-pendingapprovalid-from-tss-message-signing branch from 913b508 to f10bf80 Compare October 5, 2026 04:02

nvjsr requested review from mohammadalfaiyazbitgo, venkateshv1266 and zahin-mohammad October 5, 2026 04:12

nvjsr force-pushed the nehakumari/defi-1014-sdk-core-surface-pendingapprovalid-from-tss-message-signing branch from f10bf80 to 7d43929 Compare October 5, 2026 04:15

Navigation Menu

feat(sdk-core): surface pendingApprovalId from TSS message signing - #9858

feat(sdk-core): surface pendingApprovalId from TSS message signing#9858
nvjsr wants to merge 1 commit into
masterfrom
nehakumari/defi-1014-sdk-core-surface-pendingapprovalid-from-tss-message-signing

nvjsr commented Sep 30, 2026 •

edited

Loading

Uh oh!

linear-code Bot commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

mullapudipruthvik commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026 •

edited

Loading

Uh oh!

mohammadalfaiyazbitgo Oct 1, 2026

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

nvjsr Oct 5, 2026

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

nvjsr Oct 5, 2026

Uh oh!

zahin-mohammad Oct 1, 2026

Uh oh!

nvjsr Oct 5, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

nvjsr commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

Problem

Changes

Test evidence

Dependencies / ordering

Contract

Uh oh!

linear-code Bot commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mullapudipruthvik commented Sep 30, 2026

Uh oh!

github-actions Bot commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review for TSS Message Signing Pending Approval Changes

Overall Assessment ✅

Detailed Review

🟢 Strengths

🟡 Minor Observations

🟢 Security Assessment

🟢 Code Quality

Recommendation: APPROVE ✅

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

nvjsr commented Sep 30, 2026 •

edited

Loading

github-actions Bot commented Sep 30, 2026 •

edited

Loading

github-actions Bot commented Sep 30, 2026 •

edited

Loading